-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add power off/on events to automate control and the foreign key to events physical server #15138
Conversation
# | ||
# Physical Servers Operations | ||
# | ||
phs_poweroff,PHS Power OFF,Default,phs_operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blomquisg @juliancheal I think it makes sense to separate physical and virtual power operations. We prefix the vm operations with vm
. Is there a commonly used prefix you would suggest here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @blomquisg @juliancheal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phs
seems good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion the options are ph, phs or ps.
app/models/event_stream.rb
Outdated
@@ -26,6 +26,7 @@ class EventStream < ApplicationRecord | |||
belongs_to :container_node | |||
|
|||
belongs_to :middleware_server, :foreign_key => :middleware_server_id | |||
belongs_to :physical_server, :class_name => "PhysicalServer", :foreign_key => :physical_server_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems not necessary to specify :class_name
and :foreign_key
.
@gmcculloug is this PR good to merge? |
# | ||
phs_shutdown,Physical Server Shutdown,Default,phs_operations | ||
phs_start,Physical Server Start,Default,phs_operations | ||
phs_reset,Physical Server Reset,Default,phs_operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through this file again I have to ask why we are abbreviating these at all? The existing pattern in this file would lead me to say the event names should be: physical_server_*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you @gmcculloug, I use phs because of the suggestions. But this acronym may confuse new contributors for not being widespread or used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodneyhbrown7 @juliancheal @lfu @gmcculloug @blomquisg I will change the events names ok? All agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is fine.
Done |
@@ -210,3 +210,10 @@ containerreplicator_successfulcreate,Replicator Successfully Created Pod,Default | |||
containerreplicator_compliance_check,Replicator Compliance Check,Default,compliance | |||
containerreplicator_compliance_passed,Replicator Compliance Passed,Default,compliance | |||
containerreplicator_compliance_failed,Replicator Compliance Failed,Default,compliance | |||
|
|||
# | |||
# Physical Servers Operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be # Physical Server Operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I didn't see this. Done
Checked commits AndreyMenezes/manageiq@2b04d09~...25142c9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@gmcculloug @lfu It's ok now? |
This PR make the following changes: